Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: adding more required functionality #5

Merged
merged 7 commits into from
Jan 3, 2024
Merged

chore: adding more required functionality #5

merged 7 commits into from
Jan 3, 2024

Conversation

priteshbandi
Copy link
Contributor

@priteshbandi priteshbandi commented Dec 23, 2023 •
edited
Loading

  • Adding more functionality required for notation-go to depend on this pkg.
  • Also some renaming and update to Error struct to matching it with notation-go.

@priteshbandi priteshbandi mentioned this pull request Dec 23, 2023
Closed
Two-Hearts
Two-Hearts reviewed Dec 26, 2023
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please add doc to struct CLI defined at line 23 of cli.go.
  2. When I new a CLI struct with pl == nil: cli, _ := New("notation-my-plugin", nil), this will panic when I call cli.Execute(...). We should always check nil when new a CLI.
  3. In NewWithLogger function, we are only checking prefix of the plugin executable name. We should also check the suffix matches with the plugin's metadata.Name, i.e. {plugin-name} of notation-{plugin-name} should equal to pl.GetMetadata resp.Name. In fact, do we really need the executableName parameter here?
  4. deferStdout() and type discardLogger struct{} should be moved to utils.go
  5. In utils.go getValidArgs(), the result strings should not be hardcoded, because they are defined in the proto.go:

    notation-plugin-framework-go/plugin/proto.go

    Line 27 in c8c68d2

    type Command string

example/main.go Outdated Show resolved Hide resolved
example/main.go Outdated Show resolved Hide resolved
plugin/proto.go Show resolved Hide resolved
priteshbandi and others added 2 commits December 25, 2023 21:16
@priteshbandi @Two-Hearts
Update main.go
9fc7da6 
Co-authored-by: Patrick Zheng <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
@priteshbandi @Two-Hearts
Update main.go
511a017 
Co-authored-by: Patrick Zheng <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
@priteshbandi priteshbandi force-pushed the main branch 2 times, most recently from ff76a38 to 591e3f3 Compare December 30, 2023 09:10
@priteshbandi
PR feedbackl
d629628 
Signed-off-by: Pritesh Bandi <[email protected]>
@priteshbandi
Copy link
Contributor Author

  1. Please add doc to struct CLI defined at line 23 of cli.go.

    1. When I new a CLI struct with pl == nil: cli, _ := New("notation-my-plugin", nil), this will panic when I call cli.Execute(...). We should always check nil when new a CLI.

    2. In NewWithLogger function, we are only checking prefix of the plugin executable name. We should also check the suffix matches with the plugin's metadata.Name, i.e. {plugin-name} of notation-{plugin-name} should equal to pl.GetMetadata resp.Name. In fact, do we really need the executableName parameter here?

    3. deferStdout() and type discardLogger struct{} should be moved to utils.go

    4. In utils.go getValidArgs(), the result strings should not be hardcoded, because they are defined in the proto.go:

      notation-plugin-framework-go/plugin/proto.go

      Line 27 in c8c68d2

      type Command string

Done. Addressed all the feedbacks

@priteshbandi
feedback
d1bf254 
Signed-off-by: Pritesh Bandi <[email protected]>
Two-Hearts
Two-Hearts reviewed Jan 2, 2024
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a single nit comment.

cli/cli.go Show resolved Hide resolved
JeyJeyGao
JeyJeyGao reviewed Jan 2, 2024
View reviewed changes
cli/utils.go Outdated Show resolved Hide resolved
JeyJeyGao
JeyJeyGao reviewed Jan 2, 2024
View reviewed changes
cli/cli.go Outdated Show resolved Hide resolved
@priteshbandi
feedback 2
21b2682 
Signed-off-by: Pritesh Bandi <[email protected]>
@priteshbandi
feedback 3
3e42857 
Signed-off-by: Pritesh Bandi <[email protected]>
Two-Hearts
Two-Hearts approved these changes Jan 2, 2024
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

rgnote
rgnote approved these changes Jan 3, 2024
View reviewed changes
Copy link

@rgnote rgnote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

JeyJeyGao
JeyJeyGao approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@priteshbandi priteshbandi merged commit c077eda into notaryproject:main Jan 3, 2024
1 check passed
@priteshbandi priteshbandi mentioned this pull request Jan 29, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shizhMSFT shizhMSFT shizhMSFT left review comments

@Two-Hearts Two-Hearts Two-Hearts approved these changes

@JeyJeyGao JeyJeyGao JeyJeyGao approved these changes

@rgnote rgnote rgnote approved these changes

@gokarnm gokarnm Awaiting requested review from gokarnm gokarnm is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@priteshbandi @rgnote @shizhMSFT @JeyJeyGao @Two-Hearts